Skip to content

Latest commit

 

History

History
59 lines (54 loc) · 3.12 KB

036.md

File metadata and controls

59 lines (54 loc) · 3.12 KB

shealtielanz

medium

The lastCompletedMigration can be set by anyone who is not owner which will lead lack of Persistent interdependence.

Line of Code Lines of Code

Summary

Anyone that is not the owner can call the setCompleted function to manipulate the lastCompletedMigration due to the restricted() modifier doesn't work as expected by the developers.

Vulnerability Detail

Although the Migrations contract is not necessarily stated as in scope, and this submission might not reach the developers, it is said in the contract that.

/*NOTE: Migrations contract implements Truffle interface for marking last completed migration,
        but also is enhanced to store (retrievable only by deployer) information map, used for
        retrieving addresses of deployed contracts for providing persistent interdependence.

        Therefore, keeping address of contract in build json for Truffle for every network is crucial
        to keep things consistent.
*/

meaning this contract helps in keeping things consistent in the protocol as a whole, the issue arises from the fact that the restricted() modifier is appended to the setCompleted to insure that only the owner would be able to set the lastCompletedMigration to store and keep track of information map, but the modifier doesn't implement that idea'

    modifier restricted() {
        if (msg.sender == owner) _;
    }

as seen above, that check doesn't prevent anybody apart from the owner from calling a function with this modifier, I did a test on remix.ide for this and was able to call the setCompleted function with any other address, the result is below.

	true Transaction mined and execution succeed
transaction hash	0x3114c1e2e82177ad175075d35097bfa31296c28bb1a886562eea55fe75c51e81
from	0x4B20993Bc481177ec7E8f571ceCaE8A9e22C02db
to	Migrations.setCompleted(uint256) 0xd9145CCE52D386f254917e481eB44e9943F39138

the transaction succeeded although that address wasn't the owner's.

Impact

The consistency of the protocol might be corrupted as any attacker aware of this can manipulate the lastCompletedMigration with little or no cost.

Code Snippet

https://github.com/USSDofficial/ussd-contracts/blob/f44c726371f3152634bcf0a3e630802e39dec49c/contracts/Migrations.sol#LL16C1-L18C6

Tool used

Remix.IDE Manual Review and Remix.IDE

Recommendation

rewrite the modifier like this instead.

   modifier restricted() {
       require(msg.sender == owner, "not owner");
        _;
    }

As I have tested it and the call to the function setCompleted fails if it is called by anyone who is not the owner.

false Transaction mined but execution failed
transaction hash	0xd16ff9b1263299e6f9c789a9f6fff421ce77ac89d15b36121d3bd4fea6ecf984
from	0x5B38Da6a701c568545dCfcB03FcB875f56beddC4
to	Migrations.setCompleted(uint256) 0x652c9ACcC53e765e1d96e2455E618dAaB79bA595